Conversation
anarchivist
left a comment
anarchivist
left a comment
There was a problem hiding this comment.
generally looks good - rw+c. most of my comments are minor. you might want to ask Anna to look at this, too.
Gemfile.lock
Outdated
|
|
||
| BUNDLED WITH | ||
| 2.5.22 | ||
| 2.5.3 |
There was a problem hiding this comment.
can we make sure we're using a more recent version of bundler here? it certainly should be no older than 2.5.22
There was a problem hiding this comment.
Strange. 2.5.22 specified in the docker file..., I'm not sure how the lock file got downgraded. I'll try to bump this to the same bundler version we're using for Framework I guess... 2.7.2
awilfox
left a comment
awilfox
left a comment
There was a problem hiding this comment.
This looks mostly good, just a few nits. However, it looks like rails app:update wasn't run, similar to what happened at the beginning of BerkeleyLibrary/UCBEARS#21. Can you make sure that's run and the configuration files are updated appropriately? I described how to do that in the UCBEARS MR.
| ActiveRecord::Base.connection_pool.with_connection(&:active?) | ||
| rescue PG::ConnectionBad |
There was a problem hiding this comment.
When we upgraded Framework to 8.0, the suggested way to do it was to use verify! on the connection object. We also needed to add ActiveRecord::DatabaseConnectionError to the rescue block, but I'm not sure if that's because of verify! or because in general Rails 8 can raise that.
| ActiveRecord::Base.connection_pool.with_connection(&:active?) | |
| rescue PG::ConnectionBad | |
| ActiveRecord::Base.connection.verify! | |
| rescue PG::ConnectionBad, ActiveRecord::DatabaseConnectionError |
I don't mind doing it the way already here; I just haven't seen it before. (will this raise if there are no active connections, or just return nil?)
|
|
||
| RUBY VERSION | ||
| ruby 3.3.9p170 | ||
| ruby 3.3.9p170 |
There was a problem hiding this comment.
Should we be moving this to 3.3.11 as well?
| gem 'rack-cors' | ||
| gem 'rails', '~> 7.0.4' | ||
| gem 'ransack', '~> 2.6' | ||
| gem 'rails', '~> 8.0.4' |
There was a problem hiding this comment.
They've already snuck out another release while this was being worked on!
| gem 'rails', '~> 8.0.4' | |
| gem 'rails', '~> 8.0.5' |
| - name: Validate database migrations | ||
| env: | ||
| RAILS_ENV: production | ||
| SECRET_KEY_BASE: dummy |
There was a problem hiding this comment.
Do we want to test the Rails secrets stuff here like we do in Framework?
See BerkeleyLibrary/framework@6cf75564ef, specifically .github/workflows/build.yml and docker-compose.ci.yml. This makes a "real" (throwaway) secret, and that ensures the app is going to read it from the correct place in production.
3 participants
Upgrade Rails and dependencies to 8.0.x